Python: Fix reasoning content parsing in OpenAIChatCompletionClient#7028
Python: Fix reasoning content parsing in OpenAIChatCompletionClient#7028giles17 wants to merge 6 commits into
Conversation
Fix two issues with reasoning content handling in the Chat Completions client: 1. (microsoft#6979) reasoning_details plaintext buried as encrypted data: The client dumped the entire reasoning_details array into Content.protected_data without setting Content.text, causing AG-UI to emit ReasoningEncryptedValueEvent instead of visible ReasoningMessageContentEvent for plaintext reasoning providers (e.g. OpenRouter). Now extracts readable text from reasoning_details entries into Content.text while preserving protected_data for round-trip fidelity. 2. (microsoft#6978) Mistral list content causes crash: Mistral reasoning models return content as a list of typed chunks ([{"type": "thinking", ...}, {"type": "text", ...}]) instead of a plain string. _parse_text_from_openai assumed content was always a string, causing a Pydantic ValidationError downstream. Now detects list content and parses thinking chunks as Content.from_text_reasoning and text chunks as Content.from_text. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Python Test Coverage Report •
Python Unit Test Overview
|
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Pull request overview
This PR fixes parsing of “reasoning” outputs in the Python OpenAIChatCompletionClient so downstream consumers (notably AG-UI) can display plaintext reasoning correctly and avoid crashes when providers return structured chunked content.
Changes:
- Add
_extract_reasoning_text()and use it to populateContent.textfor plaintextreasoning_detailswhile preserving full payload round-tripped inprotected_data. - Refactor
_parse_text_from_openai()to returnlist[Content]and introduce_parse_chunked_content()to handle Mistral-stylecontent: [...]chunks (thinking+text). - Add regression tests covering plaintext
reasoning_detailsextraction and chunked list-content parsing in both streaming and non-streaming paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| python/packages/openai/agent_framework_openai/_chat_completion_client.py | Adds plaintext reasoning extraction + chunked content parsing; updates response parsing to emit correct Content items. |
| python/packages/openai/tests/openai/test_openai_chat_completion_client.py | Adds tests for plaintext reasoning extraction and Mistral chunked content in both streaming and non-streaming modes. |
- Use cast() for proper type narrowing in _extract_reasoning_text and
_parse_chunked_content to satisfy pyright strict mode
- Handle {"content": "..."} string shape in _extract_reasoning_text
(addresses review comment about missing format coverage)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
model_construct bypasses Pydantic runtime validation but mypy still checks declared types. Use cast(Any, ...) for the list content args. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add 'summary' field extraction in _extract_reasoning_text for
reasoning.summary entries from OpenRouter
- Handle message.reasoning and message.reasoning_content top-level
fields (plaintext reasoning without reasoning_details) in both
streaming and non-streaming paths
- reasoning_details takes priority when both fields are present
- Preserve original Mistral chunk list in additional_properties
('_source_content_list') so _prepare_message_for_openai can
reconstruct the structured list content for multi-turn reasoning
- Add 5 new tests covering all new behaviors
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove leading underscore from _skip_structured_siblings variable since it is accessed (not a dummy variable). Ruff's used-dummy-variable rule flags variables with leading underscores that are read. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated Code Review
Reviewers: 5 | Confidence: 76%
✓ Correctness
The PR correctly fixes both issues (#6978 and #6979) with well-structured parsing logic and thorough tests. The
_extract_reasoning_text()helper handles documented provider formats (includingreasoning.summaryentries), the fallback tomessage.reasoning/reasoning_contentfields is properly gated behind anelif, and the Mistral round-trip via_source_content_listis tested end-to-end. I found no high-severity correctness issues.
✓ Security Reliability
The PR is well-structured with proper defensive type checks and graceful handling of untrusted provider response shapes. The
_extract_reasoning_texthelper safely handles all expected formats. The_parse_chunked_contentmethod correctly validates chunk types before processing. The main concern is a minor reliability edge case in theskip_structured_siblingsmechanism in_prepare_message_for_openai, which could silently drop unrelated text/reasoning content if it follows Mistral-style chunked content in the same Message. However, given the current parsing architecture, this situation does not arise in practice. No critical security or reliability issues found.
✓ Test Coverage
The PR adds 11 new tests covering the core happy paths for reasoning text extraction, Mistral chunked content, streaming, and round-trip serialization. The tests are well-structured with meaningful assertions. However, there is a notable gap: no test verifies that opaque/encrypted reasoning_details (entries without 'text' or 'summary' fields) correctly yield Content.text=None, which is the key invariant distinguishing genuinely encrypted data from displayable reasoning. The existing test
test_parse_text_reasoning_content_from_response(line 787) tests this shape but was written before the extraction logic existed and doesn't assert thetextfield value — it now silently extracts 'summary' text where it previously had None.
✓ Failure Modes
The PR correctly fixes the two reported issues (reasoning plaintext extraction and Mistral list-content parsing). The
skip_structured_siblingsmechanism in_prepare_message_for_openaiuses a type-based heuristic that can silently drop non-siblingtext_reasoningcontent (e.g., fromreasoning_details) if it follows a Mistral chunked source without intervening tool-call items. This is unlikely with current providers but represents a latent silent-failure path. No other significant failure modes were found.
✓ Design Approach
The new reasoning/chunked-content handling fixes the reported cases, but the round-trip design still has two silent data-loss edges in
_prepare_message_for_openai(): it only recognizes_source_content_listwhen the marked content istext_reasoning, and its skip flag suppresses every later text/text_reasoning item in the same message instead of only the siblings from that structured list.
Automated review by giles17's agents
|
|
||
| # Store the original list on the first item so it can round-trip correctly. | ||
| if results: | ||
| results[0].additional_properties["_source_content_list"] = chunks |
There was a problem hiding this comment.
This round-trip marker is written onto results[0] regardless of that item's type, but the serializer only reconstructs the original structured list in the case "text_reasoning" if "_source_content_list" ... branch (lines 1068-1074). A chunk list like [{"type": "text", ...}, {"type": "thinking", ...}] is accepted by this parser, yet it will be replayed as plain assistant content instead of the original list because the marker sits on a text content and is ignored during serialization. The marker needs to be honored independently of whether the first emitted content was text or reasoning.
| # Some OpenAI-compatible providers (e.g. OpenRouter) expose plaintext reasoning | ||
| # via a top-level "reasoning" or "reasoning_content" field instead of (or in | ||
| # addition to) "reasoning_details". Surface it when no reasoning was already parsed. | ||
| elif (reasoning_str := ( | ||
| getattr(choice.message, "reasoning", None) | ||
| or getattr(choice.message, "reasoning_content", None) | ||
| )) and isinstance(reasoning_str, str) and reasoning_str: | ||
| contents.append(Content.from_text_reasoning(text=reasoning_str)) |
There was a problem hiding this comment.
This is great for surfacing reasoning from things like vLLM, but a lot of open-source models also expect the reasoning to be passed back on subsequent requests. Would be nice if this could be propagated back in the request based on the key it was seen in the response. (i.e. vLLM returns reasoning and expects reasoning on associated message in the next request)
Motivation and Context
Fixes #6978 and #6979.
Two related bugs in
OpenAIChatCompletionClientprevent reasoning content from being displayed correctly:Python: [Bug]: OpenAI Chat Completions client buries plaintext
reasoning_detailsas encrypted data #6979 - Plaintext reasoning buried as encrypted data: The client dumps the entirereasoning_detailsarray intoContent.protected_datawithout settingContent.text. Downstream, AG-UI emitsReasoningEncryptedValueEvent(opaque) instead of visibleReasoningMessageContentEventfor providers like OpenRouter that send plaintext reasoning.Python: [Bug]: OpenAI Chat Completions client crashes on (and drops) Mistral reasoning content chunks #6978 - Mistral list content causes crash: Mistral reasoning models return
contentas a list of typed chunks ([{"type": "thinking", ...}, {"type": "text", ...}]) instead of a plain string._parse_text_from_openaiassumed content was always a string, causing a PydanticValidationErrorcrash in AG-UI.Changes
_chat_completion_client.py_extract_reasoning_text()helper (module-level): Extracts readable text from variousreasoning_detailsformats (list of dicts withtextkeys, dict withcontentlist, plain strings). ReturnsNonefor genuinely encrypted/opaque data.reasoning_detailshandling (lines 753-757, 799-803): Now passes extracted plaintext asContent.textwhile preserving the full JSON inprotected_datafor round-trip fidelity._parse_text_from_openai(): Returnslist[Content]instead ofContent | None. Detects whenmessage.contentis a list and delegates to_parse_chunked_content()._parse_chunked_content()static method: Parses Mistral-stylethinkingchunks →Content.from_text_reasoningandtextchunks →Content.from_text.Tests
test_parse_reasoning_details_extracts_plaintext— verifies plaintext extraction from OpenRouter-style reasoning_detailstest_parse_reasoning_details_extracts_plaintext_streaming— same for streaming pathtest_parse_mistral_chunked_content_from_response— verifies Mistral list content parsing (non-streaming)test_parse_mistral_chunked_content_streaming— verifies Mistral list content parsing (streaming)test_parse_plain_string_content_still_works— regression test for normal string contentValidation
test_openai_chat_completion_client.pypass (including 6 new tests)_chat_client.py) which already handles reasoning correctly